Skip to content

test(ui): run the live Playwright suite in CI against the built UI image - #2874

Merged
Charlesthebird merged 25 commits into
kagent-dev:mainfrom
toreysoloio:toreysoloio/uiLiveCi
Sep 22, 2026
Merged

Charlesthebird merged 25 commits into
kagent-dev:mainfrom
toreysoloio:toreysoloio/uiLiveCi

Conversation

@toreysoloio

@toreysoloio toreysoloio commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor
  • live test coverage now implmeneted
  • moved some tests to new "shared" folder, which contains tests that run both live and with mock data
  • addressed some test-related deferred coverage issues

🤖 written by Claude (start)

The browser suite only ever ran against fixtures, so nothing checked the app an operator actually gets. playwright/live/ now runs in the test-e2e job against the image built from ui/Dockerfile — nginx, the SPA fallback and the runtime config — on the same cluster as the Go end-to-end tests.

Closes #2657
Closes #2828

What runs where

tests/ is mock-only and live/ needs a cluster; shared/ is the journeys that assert the same thing against both, one create-read-edit-delete per resource. That split is the point: a spec in shared/ may not name a fixture or a ?mock= scenario, which conventions.test.ts enforces.

The product fix in here

Every list page drew its summary on !error && !isLoading, and SWR reports isLoading false on the first paint — so a page that had not yet asked the controller anything rendered "0 of 0 harnesses" for about 600ms, measured against a real backend. Those six summaries now wait for data.

A later sweep for the same expression found two more: AgentPage's conversation count, and AppDetailPage, which announced "No such app" on that gate — an absence asserted before the read that would contradict it, under a comment claiming the opposite. Both are fixed. Neither is measured: the mock backend answers from the page's own memory, so no loading window exists to catch, and a reverted control page does not flash either. They are fixed for parity with the six and because the absence rule in playwright/README.md is unambiguous.

The deferred coverage in #2828

Blocked on Entry Where it stands
A proto change Server-side paging, searching, sorting open — substrate's landed in #2771; models, tool servers and prompts still answer with everything
Auto-titling open — and only server-side: AgentInstance carries a reader-supplied name, but nothing denormalising the first message, so titling an unnamed row still costs one read per row. The rail pays it, bounded at thirty; the table does not
Conversation search scope open, and not currently a defect — ListAgentInstancesRequest has page, all_creators, agent_template and harness, and no filter or sort field. The client follows every page token, so a browser-side search covers every row. It becomes one the moment page-following is removed, which must take the search and sort server-side in the same change
A surface that doesn't exist Agent-form validation closed, permanently — an agent is an (AgentTemplate, Harness) pair materialised by admission. Nothing creates one, so there is nothing to validate; router/routes.ts records why there is no agentNew and no agentEdit. The create-and-read-back property the removed spec carried lives in shared/harnesses/ and shared/agent-templates/, which create against either backend
An agent's readiness reason, end to end closed — not a gap: the record's failure is tests/agents/agent-details.spec.ts, the template's admission is step 5 of its own spec, read from the controller's status. No single surface shows both, by design
A fixture that doesn't exist A template labelled for the only harness open — checked against a one-harness cluster instead
A real controller Tool approval, and a turn parked without the HITL extension done — the controls shipped in #2714 and nothing drove them; tests/chat/approvals.spec.ts does, reading the decisions back off the reply rather than off the form
Streaming open — and misfiled on the ticket: the live lane exists now, so what is missing is a cluster with a model that can answer
The suspending stage not planned — the reading has exhaustive unit coverage where the logic lives, including the case that matters most (no stage claimed when a turn ends). A browser test would add that a controller string reaches an element, for two stages of four, and would be the only spec here needing its own API key
A convention amendment A broken create aborts that resource's failure steps closed — not a deferral at all. README.md states the ordered lifecycle as a deliberate trade and the right way round, so the entry was an open item against a decision already made. Dropped rather than left contradicting it

DEFERRED.md went from 409 lines to 298, because most of it had become a record of work already finished. Everything settled is now one line each in a Closed section — onboarding, a cleanup spec, the REST path tests, the extension-point specs, the mock ports, the live suite running in CI, and AgentDetailsPage, which the file still called uncovered after this branch covered it.

The two API-blocked entries above say which of them is a defect and which is not, and quote what the proto actually carries rather than what the file remembered. It now also says which of three states an entry is in — deferred, not planned, or closed — because "we are not going to do this, and here is why" is a different thing from "nobody has got to it yet", and only one of them should be picked up by the next person reading the file.

Two specs cited sections that removal deleted. One resolves — the reason an AgentInstance record is not a reduction of the old details page stays. The other cited a sharing gap that was never actually written down: a fixture cannot prove the share header reaches a backend, and a live check needs a visitor who is not the owner, or the read succeeds regardless and proves nothing. That is an entry now.

Testing

Mock suite, no cluster needed:

  1. cd ui && yarn test:pw

Live, against a real backend:

  1. scripts/setup-cluster/setup-cluster.sh — it leaves the UI on localhost:8080.
  2. cd ui && UI_LOOP_LIVE_URL=http://localhost:8080 yarn test:pw:live
  3. Check the cluster afterwards: kubectl get agenttemplate,harness,modelconfig -A | grep e2e-live should find nothing, since each journey removes what it made.

In CI, the test-e2e job runs the same lane against the LoadBalancer address and uploads playwright-report/ on failure.


🤖 written by Claude (end)

toreysoloio and others added 20 commits September 16, 2026 12:28
`ui-tests` proves the UI is self-consistent, not that it works: it runs on the
in-browser mock, so a backend contract change cannot fail it. That is how kagent-dev#2638
emptied three Substrate fields with every check green. Closes kagent-dev#2657.

The lane runs in `test-e2e`, after the Go end-to-end tests and on the same
cluster, reusing what that job already stands up — including the `smoke` agent
the schedules spec needs. The UI image joins the fan-out, `ui.replicas` is no
longer zeroed, and the address is the `kagent-ui` service's MetalLB IP, the chart
already publishing it as a LoadBalancer. `UI_LOOP_LIVE_URL` points the suite at an
app it did not start, so what runs is the shipped artifact: nginx, the SPA
fallback, and the `env-config.js` rendered from the pod's environment at start,
none of which exists in front of `yarn dev`.

Two invariants worth stating, both learned the hard way:

- `build-ui` and `build-golang-adk` both depend on `proto-generate`, and `buf
  generate` writes `ui/src/generated` as well as `go/api/gen`. Run in parallel,
  one make writes into the directory the other is tarring as a build context, so
  it runs once up front with `-o proto-generate` on the fan-out.
- A green live run has to have been live. Against the image there is no build-time
  `VITE_API_MODE` pin, so `globalSetup` asserts `/mockServiceWorker.js` 404s —
  which says both that fixtures cannot be served and that this is the built
  artifact rather than a dev server that would pass every spec.

Turning the lane on meant fixing the suite it runs, because nothing had run it and
every spec had rotted. `agent-lifecycle.spec.ts` drove `/agents/new`, a page
removed long before, for an agent nobody creates — replaced by
`agent-templates.spec.ts`, which asserts the property only a cluster can settle:
admission is read from the template's status and cannot be computed in the
browser. `schedules.spec.ts` drove a dialog for an editor that is a page, and
named CI's fixture rather than `setup-cluster.sh`'s, so it could only pass where
nothing ran it. `substrate.spec.ts` read a tile once, catching the em-dash drawn
before the data lands.

Two harness faults came out of the same run: `selectFirstOption` could resolve an
option from a dropdown that had just been dismissed, and the live project now runs
`workers: 1`, these specs sharing one cluster where every mock test owns a backend
in its own page's memory.

Verified against a Kind cluster running the image built from this checkout: 7 live
specs pass, and the mock suite is unchanged at 159.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
A create, an edit and a delete are where a fixture and a controller most easily
disagree — a create wrapped one way in the fixtures and another by the API, a name
sent where a ref belonged. Those journeys ran only against the fixtures, which
answer whatever they were taught.

`playwright/shared/` is claimed by every project, mock and live alike, and holds
the portable half: the write journey for models, prompts and harnesses, plus two
properties true whatever the backend holds — no conversation listed by a bare id,
and a deep link resolving on a cold load. It is laid out like `tests/`, one folder
per resource and app-wide specs at the root, and `conventions.test.ts` enforces
that along with the rule that makes it work: a shared spec may not reach for
`?mock=`, `withScenario` or `helpers/mockCalls`.

Three invariants earned the hard way, each recorded where it applies:

- **Counts are relative, never absolute.** The fixtures seed four models and a
  cluster seeds whatever it was installed with, so `before + 1` is the only form of
  the claim that survives both — and it is exactly as strong.
- **antd draws its loading placeholder as a `tbody tr`.** Counting those read a
  seeded set of one for a table that had not loaded; data rows are
  `tbody tr.ant-table-row`.
- **A one-shot read is not an assertion.** Two specs failed reading a value while
  it was still arriving, in opposite directions — the dashboard card against the
  mock, a substrate tile against the cluster. Both poll now.

The duplicated driving is gone from `tests/`: those specs keep what only fixed data
can settle — the seeded rows, the refresh counts, the required-field marks, and the
empty and failure states no cluster can be asked for — and their titles say so.
`routing` and `dashboard` are split the same way, with the SPA-fallback steps moving
to `shared/` because nginx and Vite answer a deep link differently and only one of
them ships.

MCP servers are deliberately absent: that page cannot read its own writes against a
real backend (kagent-dev#2849), and a shared spec would have had to press Refresh to get past
it. `DEFERRED.md` records the deferral and what closing it looks like.

`live/helpers/live.ts` keeps only what is live-specific. `loadApp`,
`expectNoLoadFailure`, `rowNamed`, `dataRows` and `throwawayName` moved to
`helpers/app.ts`; the first adds the mock scenario only where there is a mock
backend to read it, and the two row helpers had drifted to different selectors in
the two suites for no reason anybody could name.

Verified against a kind cluster running the image built from this tree: 12 live
specs and 169 mock specs pass, and the cluster is left with no stray resources.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
…he reload

`agent-templates` and `schedules` were the two resources whose journey still ran
twice — once against the fixtures in `tests/`, once against a cluster in `live/` —
which is the duplication the previous commit removed everywhere else.

Both move to `shared/`, and what is left behind in each place is what only that
place can answer.

**Schedules split on a reload.** Moving the live spec wholesale did not work, and
the reason is worth keeping: step 4 reloads, and the fixture backend holds writes
in the page's own memory, so a reload starts a backend that has never heard of the
schedule. That reload is not incidental — it is the whole claim, since everything
short of a re-read could be the form showing itself its own draft. So the journey
is `shared/schedules/`, clicking through on either backend, and
`live/schedules.spec.ts` keeps only the persistence: a fractional timeout stored as
seconds plus nanos, and a named IANA zone, both still there after a reload. The
general rule is now in the README — a spec whose claim is "the backend stored it"
needs a backend that stores.

**Agent templates gained the edit they never had.** The shared spec created and
deleted but never updated, so no suite was asserting an update against a real
controller. It now changes the description in place and reads it back off the page,
and it picked up the namespace-narrowing assertion the mock create was making —
the line that once carried two faults at once, a redirect with no query string and
a caller sending `namespace` where the list narrows on `ns`.

The mock specs keep what needs fixed data: the seeded rows, the cadence and history
of a schedule and the pausing that is unique to it, the template list's narrowing
and sorting, the form's refusal of an unusable template, both branches of the delete
warning, and the empty and failure states. Their titles say what they now cover.

One thing the trim exposed: the agent-template delete was doing double duty, since
removing the template also navigated back to the list, and the step after it relied
on that. The hop is explicit now.

The create stays in the mock template spec as setup rather than as a claim. Removing
it would mean pointing the read-and-edit steps at the only seeded template that has
a harness, and a later step asserts that template's description unedited — the note
on `CREATED` records that, so the trade is not rediscovered.

Verified against a kind cluster running the image built from this tree: 13 live
specs and 173 mock specs pass, and the cluster is left with no stray resources.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Moving each write journey to `shared/` left five headers describing a spec that no
longer exists, and three were stale beyond the opening line:

- `agent-templates` said the created template "survives all the way to the delete
  that removes it" — that delete moved two commits ago.
- `harnesses` carried both "There is no update half" and "Nor is the write journey
  here", two ways of saying the same thing once the trim had landed.
- `routing` said the login route needs something to log in to. It does not: the
  route was tried against a deployment and passes. It stays on the fixtures for a
  different reason, which the header now gives — `shared/routing` already proves the
  SPA fallback on `/substrate` and an unknown path, so `/login` earns nothing by
  running twice.

Shorter as well as truer, 130 lines out for 65 in. What each keeps is the knowledge
that cannot be recovered from the code — why a prompt save replaces the whole
ConfigMap `data` map, why `ready: false` must not be drawn as "broken", why a
schedule's history outlives the schedule, and the cluster observation behind the
agent-template admission property. What went was boilerplate: every one of them
repeated that a video and a trace are recorded per test, which `README.md` says.

Comments only; no assertion, selector or title changed. 173 mock specs pass.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Six defects, most of them only reachable against a backend slower than the
fixtures — which is to say, only in CI.

**Counts were of rows on screen.** The lists page at 25, so on a cluster holding
more than a page the created row is on page two: present, correct, and invisible to
a locator. The totals now come off the `<list>-summary` line, and `searchList`
narrows to the name this run invented so the row is found wherever it landed. The
harness tab keeps counting rows, which is right there — it sets `pagination={false}`.

**`LIFECYCLE_TIMEOUT` was the tightest budget in the live run.** A
`describe.configure` overrides the per-test default, so a shared lifecycle asking
for sixty seconds was undercutting live's own hundred and twenty. It is 180s live,
60s mock.

**Cleanups skipped their deletes.** A `count()` taken straight after a navigation
reads zero because the page has not answered yet, so "is it still there?" always
said no and the resource stayed on the cluster. `appeared()` waits first, and
returns `false` rather than throwing because a throw in a `finally` replaces the
failure the test was reporting. It treats a timeout as an answer and anything else —
a locator matching several, raising strict mode — as a broken check, said out loud
rather than silently. A mistyped test id cannot be told apart and the comment says so.

**The uploaded CI artifact was near-empty.** `github` writes annotations and no
files, so the report path had nothing in it and `if-no-files-found: ignore` hid that.
The config asks for `html` alongside it.

**`LIVE_EXTERNAL_URL` was read three inconsistent ways** — `??`, `!== undefined` and
truthiness — so an empty string meant "deployed" to one and "not deployed" to
another: it would have pointed `baseURL` at `""`, checked that for nginx, *and*
started a dev server nothing would visit. Normalised at the source.

**The one-harness admission read was a race.** Those labels are applied in an effect
that lands after the buttons first render, so a single read could take the pre-effect
state and go down the "nothing admits it" branch on a cluster where something does.
The button's disabled state is the settled signal in both directions.

Two more worth stating. The live admission assertion leaned on the controller having
reconciled before the navigation — that page fetches once on mount, so a longer
timeout only re-read a DOM that was never going to change; the reload is the refetch
now. And widening the ESLint spec rules from `tests/` to `{tests,shared,live}` found
`live/pages` and `live/substrate` importing from `@playwright/test`, so neither had
the console-error guard, in the only suite that talks to a cluster.

CI gains a gate on the `smoke` template existing. It is applied part-way through the
Go step, so a step that died before that apply would fail the browser journeys three
times each against a cluster with no agent to schedule.

Verified against a kind cluster running the image built from this tree: 13 live and
173 mock specs pass, with no stray resources left behind.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
An audit of what each test actually asserts, rather than whether it is green.
Seven passed while claiming more than they checked.

The live sweep's error check ran before the page had asked the controller
anything, so its claim was "the shell rendered". Gating on each page's summary
was not enough either: it renders for a frame before the answer lands, and a
read delayed 1.5s and then failed passed both orders while the page ended up
saying "Could not load agents · HTTP 500". A networkidle gate never settles,
SWR retrying the failed read. So the sweep watches responses instead — any
non-2xx under /api/, asserted per page and again at the end, which is the one
that catches a failure landing after its page was checked.

Stale claims in the same file: `not.toContainText("__NS__")` under "resolves
its model", a string this repo has never rendered, on a table that has had no
model column since the rewrite; and a "tool count" step that only re-checked
`agents-error`. They assert the harness each row runs on, the provider tag, and
the summary's numbers rather than the static word "server".

`shared/harnesses` asserted that an unpinned image is refused while the
snapshot location was also empty — the button was disabled either way, as the
next line proved by leaving it disabled with a valid digest. It now fills a
complete draft, asserts the button is enabled, and breaks one field at a time.
The "admits no templates" warning is asserted to appear, which nothing did.

`shared/dashboard` ties its label loop to the state the poll settled on: a
clean cluster has no conversations, so in CI the loop body never ran.

Four guards named test ids that have never existed in src, and so could not
fail however the control came back. They read the chrome's own ids, search the
dialog by role, and assert there is no switch on the page. Substrate names its
five tiles rather than one invented absence.

Verified by mutation, each reverted: a create-looking id added to the header
fails the shell guard, widening the dialog regex to a control that is there
fails on count 1, and the induced late failure now fails the sweep in 2.2s.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
…from nothing

Six findings from the review of the previous commit, each verified before and
after.

The live sweep watched for a failure the controller does not produce. The app
speaks gRPC-Web, so a refused read answers HTTP 200 and puts the reason in a
`grpc-status` trailer — measured against a cluster, a missing library came back
`200 ok=true grpc-status=5`. Only nginx's own 502s would ever have reached the
non-2xx check, so it reads both now.

`pressUntil`'s budget was shorter than one attempt in the live project, where
`settled` is an assertion on a thirty-second `expect` timeout. Measured: a 20s
attempt under a 15s budget runs exactly once, under 90s it runs five times. So
every live Delete was a single press with no retry, which is the whole reason
that helper exists, and a swallowed one left the resource on the cluster.

Five cleanups called `loadApp`, `searchList` or `page.goto` inside a `finally`,
all of which throw — replacing the failure the test was reporting and skipping
the delete underneath. They go through `helpers/cleanup.ts`, which warns.
`appeared` was written for this and the blocks around it were not.

`live/schedules.spec.ts` is a journey on the global budget while every shared
one takes `LIFECYCLE_TIMEOUT`; the agent-templates journey read its row off an
unnarrowed list that pages at 25, which is what `searchList` exists for; and
six `expectNoLoadFailure` calls sat where they always pass. The helper now says
what it must be called after.

Verified on a cluster built from nothing: 13 live, 173 mock, 498 unit. The
fresh run exercised what the old one could not — the dashboard's empty branch
(no conversations, so the naming claim is untested there and now says so), the
schedules empty-state gate, and lists of one.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Second review round, five findings, each checked before acting on it.

The live admission poll could not retry. `expect.poll` calls its callback
outside its own try/catch, so a throw ends the poll rather than failing one
round — measured: a callback that throws twice then succeeds gives up after
4ms, where one that returns is retried to its deadline. Both `textContent` on a
page still drawing its Skeleton and `expectSettled` after the reload throw, so
the ninety-second budget and its message were decoration. The callback now
returns the reason it could not answer.

The harnesses cleanup was the last `finally` still calling `loadApp` and
`confirmDelete` directly, which is what `helpers/cleanup.ts` exists to stop:
thrown from there they replace the failure the test was reporting and skip the
delete underneath.

`expectNoLoadFailure` matched `-error` only, while four pages say a failed read
with `-unavailable` instead — the dashboard's recent list, the schedules table,
the tools chart, the schedule history. Each of those pages happens to draw its
`-error` sibling from the same condition, so nothing was slipping through
today; matching both means the claim no longer rests on that.

Per-assertion budgets were sixty seconds against a mock `LIFECYCLE_TIMEOUT` of
sixty, so an assertion could never exhaust its own budget and a broken one was
reported as "Test timeout exceeded" rather than by name. `READ_TIMEOUT` sizes a
read below the journey that contains it.

And the CI gate: five UI steps hung on one un-retried `kubectl get`, where a
transient hiccup skipped the whole browser lane with the job still green — the
shape of kagent-dev#2638. It retries, and a lane that does not run says so with a warning
annotation.

Verified: 13 live against a cluster built from nothing, 173 mock, 31
conventions. A dashboard whose read was aborted now fails the sweep.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Every list page drew its summary on `!error && !isLoading`, and SWR reports
`isLoading` false on the first paint — its fetcher runs in an effect, after it.
So a page that had not yet asked the controller anything rendered "0 of 0
harnesses" and held it there. Measured on the harnesses tab at 300ms intervals:
"0 of 0" for the first 600ms, then "4 of 4".

A count of nothing is a claim, and until the read lands the page has not earned
it — the same distinction this codebase already makes about a failed read,
which must not be drawn as "there is nothing here". The summary now waits for
`data`, so the six lists say nothing until they have something to say.

Found by the live suite: a journey that counted the tab before creating a
harness read 0 against a tab holding four, and then asserted "one more than
nothing".

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
…CI for what it has not got

Third review round, four findings, each measured before acting.

The tool-server assertion could not pass in CI. The e2e job installs with
`--set kagent-tools.enabled=false --set grafana-mcp.enabled=false`, and the
chart's only RemoteMCPServer is gated on the first, so that cluster has none —
where `setup-cluster.sh` leaves the default and has two. "At least one server",
added last round to stop the assertion being vacuous, would have failed every
CI run while passing on a laptop. Both counts are now required to be numbers
the page computed, and the per-server check runs only where there are servers.

Cleanup moved out of `finally` and into `afterEach`. Measured with a
four-second test: after a timeout the body's `finally` is refused with "Target
page, context or browser has been closed" while a hook still drives the page.
A timed-out journey is the likeliest live failure and was the one case these
blocks could never handle — so every resource they were meant to remove stayed
on the cluster.

Three journeys waited for a row before counting, which quietly required the
cluster to already own one of that resource; a shared spec may not assume that.
They read the total off the summary, which renders at "0 of 0" for a read that
succeeded — and which now only renders once the read has landed, the flash that
exposed being fixed in the commit before this one. The total is still read
twice and has to agree with itself.

And `expectListTotal`, `expectListLoaded` and `appeared` kept their hardcoded
sixty seconds while their callers moved to `READ_TIMEOUT`, so a stuck read
still reported the test timeout rather than naming what it waited for.

Verified: 173 mock across both engines, 13 live against the fresh cluster
running an image rebuilt from this branch, 498 unit, 31 conventions.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
A `/* … */` written among a JSX element's children is a text node, not a
comment. The gating comment added in 8bae718 landed inside `trailing={…}` on
three pages, where it is a comment, and among a `<Space>`'s children on the
other three, where it is not — so Agents, Templates and Harnesses each rendered
a paragraph about SWR into their filter bars.

Nothing caught it: `tsc` and eslint both accept it, and no assertion in this
suite read that region. 173 mock, 13 live and 498 unit tests all passed over
it.

So the pages are now asked directly, in `app-shell`, over all six surfaces
including the two tabs — two of the three were tabs. The check waits for each
page's table before asserting the absence, which is the difference between a
check and a formality: asked on arrival it passed over a harnesses tab broken
on purpose, because "there is no source on this page" is true of a page that
has not drawn yet. With the wait it fails, naming the route.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Two of the fixes in this branch reached around a tool instead of for it.

`expect.poll` ends on a throwing callback, so the live admission check wrapped
its callback in a try/catch that returned a string to keep the poll alive.
`toPass` is the primitive for retrying a callback that throws — `pressUntil`
was already using it two files away. The failure is now the assertion's own
rather than a sentence composed to survive the wrong mechanism.

And the list total was read twice and required to agree, because every summary
drew "0 of 0" for the 600ms before its read landed. That is fixed where it
belongs now, so the double read was a test carrying a workaround for a bug that
no longer exists. One read again; a return of the flash fails the journeys that
count before and after a create, which is how it was found.

Two rules added to the README for the pattern behind most of this branch's
defects: never assert an absence before the thing could appear, and clean up in
a hook rather than a `finally`.

Verified: 173 mock, 13 live, 31 conventions.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Three conflicts, each resolved toward the side that still holds:

- `ci.yaml` timeout: main raised its own to 40; the UI image and browser suite
  add about ten, so 50.
- `ci.yaml` builder and cache: main's, whole. It now reuses Blacksmith's
  persistent builder rather than creating one, which supersedes the
  create-once-before-the-fan-out care this branch added, and it dropped the
  per-image gha cache with it. `-o proto-generate` stays: that is about two
  builds writing `ui/src/generated` at once, not about caching.
- `live/schedules.spec.ts`: this branch's. Main's is the older whole-journey
  version driven by labels; here the journey lives in `shared/` and this file
  keeps only the claim a reload can settle.
- `tests/substrate/inventory.spec.ts`: this branch's named tile set, which
  catches an ate-api tile returning under any id, where main asserts one absent.

`live/agent-lifecycle.spec.ts` stays deleted: this branch moved that journey to
`shared/agent-templates/`, and main has not touched the file since the base.

Verified on the merged tree: 183 mock across both engines, 530 unit, lint and
typecheck clean. The live lane is not verified here — main moved substrate from
0.0.29 to 0.2.0-beta2, so the cluster this branch was tested against no longer
matches what CI installs.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
The merge took this branch's `live/schedules.spec.ts` over main's, which
asserted two things nothing else here did. Both are answerable on either
backend, so they land in the shared journey rather than where they came from.

A blank time zone is not blank — the form stores `timeZone || "UTC"` — so the
cadence line has to say which it is. Filled, cleared, and put back, one field
at a time. With the fallback removed the line reads "Weekdays at 09:00 ()",
which is what a reader would have been left to interpret.

And a schedule that has never run says so. The page keeps three states apart —
no executions, no search match, and a failed read — and the mock spec could
only ever assert the absence of the first; a journey that creates a paused
schedule is the one place it can be produced.

Verified on a cluster rebuilt on substrate 0.2.0-beta2, which is what main now
installs: 13 live, 183 mock, no resources left behind.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Read against kagent-dev#2828, which tracks these entries and asks that each be closed or
re-argued when its blocker moves. This branch moves one of them.

Two entries said the missing coverage "belongs in `playwright/live/`". That
lane now exists and runs in CI, so on a plain reading they are ready to write —
and whoever tried would find the real obstacle underneath: an `AgentInstance`
exists only once a message has been sent, a streamed reply needs a model that
answers, and neither cluster has one. CI installs with `OPENAI_API_KEY: fake`
and `setup-cluster.sh` sets no key at all. Both entries now say so.

And the cleanup note claimed each live spec deletes what it made in a
`finally`, which is both out of date and the opposite of what was measured: a
timed-out test has a closed page, so a `finally` cannot clean up after the
failure most likely to leave something behind. They are hooks now.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Read against kagent-dev#2828. The entry deferring this said the approval controls needed
a product decision. That decision landed in kagent-dev#2714 and the controls shipped with
it — per-tool approve and reject, a reason on a rejection, a submit — and
`chat-approval` appeared nowhere in `playwright/`. Not a deferred gap: an
untested surface behind a note that had gone out of date.

Two scenarios carry it, both parking the turn at the `ChatClient` boundary the
`asks` scenarios already park it at. That is the seam the entry argued for
keeping; what stays out is wire frames, which unit tests cover over real bytes.

`approves` offers two tools so one can be allowed and the other denied in a
single submission — a single-tool fixture cannot show that the controls belong
to their row rather than to the form. The fixture reads the decisions out of the
payload it received and says them back, so the spec asserts what went to the
agent rather than what the form looked like. Checked by breaking it both ways:
sending every decision as an approval fails on the rejected tool, and dropping
the rejection reason fails on the reason.

`asks-unknown` parks on a request this build cannot render — a turn started
without the HITL extension, carrying prose and no correlation id. The page says
so and offers only the discard, and that sentence was untested too.

Verified: 186 mock, 530 unit, 31 conventions. One firefox failure per full run,
a different test each time and passing 6/6 in isolation, at load average 50-58
with the kind cluster running; it happens with these changes stashed, so it is
the contention the config documents rather than these.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
…vered

One tool is a different prompt rather than a shorter one — Approve and Reject on
the prompt itself, a rejection opening its reason with the caret in it — and the
two-tool fixture cannot reach any of it. A second scenario covers it, and the
approval surface now has no undriven control.

Then the file itself. Three sections recited what the chat, MCP server, prompt
and form-validation specs cover, which those specs describe better; they are one
paragraph now, keeping the lesson they carried — two of them had been listed as
blocked on pages that already existed, and a stale "blocked on" costs more than
no entry. The approval entry keeps its argument and loses its recital.

One correction rather than a compression: the entry on what was lost when agents
became AgentInstances says no single surface shows an agent's readiness reason
end to end. True, and neither half is covered either — `AgentDetailsPage`, the
record with `instance-state`, `instance-operation` and `instance-failure`, has no
spec at all. Found by listing every test id in `src` against every one the suite
references: 188 of 490 are never driven, and that page is the largest of them.

Verified: 36 chat specs, 530 unit, 31 conventions.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
A file whose only value is being true, checked claim by claim: every path,
identifier, spec name and request shape it names, then the prose.

Most of it holds. `ListModelConfigsRequest` really is empty and
`ListPromptTemplatesRequest` really does take only a namespace, so the paging
table is accurate; `GetSubstrateStatus` really is gone; `ListTasks` is there to
carry the auto-titling argument; the MCP entry's two `Refresh` presses are still
in the spec, so kagent-dev#2849's acceptance test still reads. Of the three files it names
that do not exist, two are the point — old specs it records as lost.

Three corrections:

- "Not started by request: app extension-point specs" said the opposite of the
  paragraph two above it, which lists them as covered. Both specs exist and run,
  and between them assert every point the app declares.
- The cleanup row still said each live spec removes what it made in a `finally`,
  corrected everywhere else in this file already.
- `agent-lifecycle.spec.ts` is named for a fault it had; it is gone, replaced by
  `shared/agent-templates/`, and now says so.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Found by listing every test id in `src` against every one the suite references:
`AgentDetailsPage` was the largest undriven surface in the app. Its fixtures were
already there — `instances.failed` is commented "with a reason the conversation's
record page shows" — so the fixture was written for this page and the spec never
followed.

Two tests. The record reports its state in words rather than in the controller's
vocabulary, says what that state means, and links to the agent and the template
instead of restating what they hold — which is the claim `DEFERRED.md` makes for
why replacing the old agent details page was not a reduction: an instance has no
spec. Then the three readings the page must keep apart: a failure carries the
reason it failed, somebody else's conversation reads as not found because an
instance is read as its creator, and a read that failed offers a retry rather
than a way back to the list.

Checked by breaking both substantive claims: pointing the agent link at the list
fails the href, and showing the wire value in the state tag fails on "ready"
where a word was expected.

Verified: 191 mock, 31 conventions. The two firefox checkpoints failures in the
full run pass 6/6 in isolation and were already shown to fail with these changes
stashed — the contention the config documents.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
…from nothing

Seven findings, none blocking, two with teeth.

`detailURL` was recorded after the heading assertion, so a create that reached
the controller while the detail page was slow to draw left the hook with no
address — a real Schedule on the cluster, which is the litter the hook exists to
prevent. Both schedules specs record the address first now.

And the fixture had grown a second payload parser that drifted permissive: it
took an empty `approvals` array and an empty `rejection_reason` where the app's
own reader rejects both, so it would have acknowledged payloads the app refuses.
It uses `readToolApprovalResponse` now.

The rest: the live sweep walks eight pages on one budget and had the project
default, so a slow cluster reported "Test timeout exceeded" rather than naming
the page; `agent-details` applied its mock scenario twice and worked only
because `URLSearchParams.get` returns the first; an orphaned comment; and two
documents each contradicting something true in the same file — the README said
`finally` eleven lines above the rule saying never `finally`, and a CI comment
claimed `buildx-create` runs once when nothing passes it `-o`.

Verified: 193 mock across both engines, 13 live against a cluster deleted and
rebuilt from nothing on substrate 0.2.0-beta2 with images from this tree, 530
unit, 31 conventions. No `e2e-live-*` left on the cluster afterwards.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 18, 2026
@toreysoloio
toreysoloio marked this pull request as ready for review September 22, 2026 15:09
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 22, 2026
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 22, 2026
The suspending and resuming lifecycle stages are not planned. The reading has
exhaustive unit coverage where the logic lives, including the case that matters
most - no stage claimed when a turn ends - so a browser test would add that a
controller string reaches an element, for two stages of four, and would be the
only spec here needing its own API key. Recorded as a decision so it is not
re-argued each time somebody notices the gap.

The broken-create entry is dropped: it contradicted README.md, which states the
ordered lifecycle as a deliberate trade and the right way round. A file of gaps
should not carry an open item against a decision already made.

DEFERRED.md now says which of three states an entry is in - deferred, not
planned, or closed - since that is most of what a reader needs from it.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 22, 2026
A sweep for the summary-flash pattern this branch already fixed on six pages
found two more call sites it had missed.

AgentPage's conversation count used !error && !isLoading, the same expression,
so it can report "0 of 0 conversations" for a page that has not yet asked the
controller anything. AppDetailPage is the worse of the two: it announces "No
such app" on the same gate, which asserts an absence before the read that would
contradict it - under a comment claiming absence is only meaningful once the
read finished.

Not measured. The 600ms flash was measured against a real backend, and the mock
backend answers from the page's own memory, so neither the slow scenario nor a
delayed route reproduces a loading window - a reverted control page does not
flash either. These two are fixed for parity with the six and because the
absence rule in playwright/README.md is unambiguous, not on fresh evidence.

Also corrected a stale comment in AgentPage: it credited a note under the table
with saying the search is client-side, and that note was removed some time ago.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 22, 2026

@Charlesthebird Charlesthebird left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This automated review is a first pass. A manual review will follow once the blockers are addressed.


Blocks merge: the shared agent-templates spec claims the mock suite still pins the delete-dialog wording, but this PR deleted that pin. The rest is non-blocking. The main theme is coverage lost when mock steps moved to shared/: schedules, the model edit form, and one vacuous absence check.

⚠️ What I checked
  • ✅ UI image pushed to the kind registry and pulled with pullPolicy=Always; LoadBalancer IP read after helm --wait
  • ✅ Port 8080 matches the chart service and nginx listen
  • ✅ mockServiceWorker.js 404s in nginx.conf:134 and is deleted in the Dockerfile
  • ✅ proto-generate runs once before the parallel builds
  • ✅ Live step has no continue-on-error; fork PRs get no secrets
  • ✅ Live workers: 1 / fullyParallel: false override the top-level config
  • ✅ Every moved spec lands in at least one lane
  • ✅ Cleanup deletes only throwawayName resources (pid + timestamp)
  • ✅ No waitForTimeout, toast or networkidle waits in shared/, live/ or new tests/ specs
  • ✅ Mock HITL payload uses the same URI-keyed shape as a2aGrpcChatClient.ts:457
  • ✅ Empty and error states kept for harnesses, models, prompts, schedules, agent-templates
  • ⚠️ Coverage parity between old mock steps and new shared journeys
  • ⚠️ Absence checks wait for a positive list signal first

🤖 written by Claude

Comment thread ui/playwright/shared/agent-templates/agent-templates.spec.ts
Comment thread ui/playwright/shared/schedules/schedules.spec.ts
Comment thread ui/playwright/tests/schedules/schedules.spec.ts
Comment thread ui/playwright/shared/models/models.spec.ts
Comment thread ui/playwright/shared/models/models.spec.ts Outdated
Comment thread ui/src/pages/AgentsPage.tsx
Comment thread .github/workflows/ci.yaml Outdated
Comment thread ui/playwright/live/substrate.spec.ts Outdated
Comment thread ui/playwright/DEFERRED.md Outdated
Comment thread ui/playwright/conventions.test.ts
Both were listed as plainly open, which read as queued coverage work. Neither
is: what each waits on is a proto change, and the behaviour as it stands is
already pinned by agent-page.spec.ts.

Auto-titling is a cost decision. Conversation search is a tripwire rather than
a gap - the client follows every page token, so a browser-side search covers
every conversation with that agent, and it stops being honest only when the
page-following goes.

Both now record what the proto actually says, checked there rather than here:
AgentInstance gained name and context_id and carries nothing derived from the
transcript, and ListAgentInstancesRequest is all_creators, page, agent_template
and harness with no filter or sort field.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 22, 2026
The summary flash was diagnosed wrong. It is not SWR running its fetcher in an
effect: useApiResource reports isLoading false for a held-back key, and the
namespace-gated hooks joined [] to "" and held back forever - so the data gate
this branch added would have hidden those summaries permanently on a cluster
with no visible namespaces. Both hooks now key on the array, so a known-empty
set resolves as the answer it is. The explanation lives in useApiResource, once,
instead of pasted into seven pages with the wrong cause in it.

Also from the review:

- tests/agent-templates asserts the populated delete warning again - the count,
  what keeps working and what cannot be started. Only the empty branch was left,
  so the shared spec's claim that the mock suite pins the wording was false.
- tests/models checks that editing does not demand the API key again.
- tests/schedules ports the fixture claims dropped with the old steps: the row's
  derived cadence, both confirmation shapes and their Keep, and that a delete
  takes one row and leaves the others. Rename-from-list is not ported: there is
  no rename on this page.
- The created flag is set before each submit, not after the redirect, so a slow
  redirect no longer fails the test and leaks the resource.
- playwright.config matches by glob. Playwright matches a testMatch regex
  against the absolute path, so a checkout under a directory called tests/ or
  live/ put every spec in the wrong lane. Both lanes verified unchanged: 96
  mock, 13 live.
- conventions.test.ts catches a shared spec importing a named fixture, which is
  the half the scenario check missed and the half the README already claimed.
- The live report uploads on any finished run: with two retries, a test that
  fails then passes left no trace.
- live/substrate checks for a load failure at the end, not after a card the page
  draws before it has read anything, and shared/schedules waits for the list
  before asserting a row is gone.
- The prompts abandoned-draft claim is dropped - a goto restarts the mock
  backend, so a draft that wrongly saved would be wiped and the test still pass.
- DEFERRED.md cited tests/harnesses for a create it does not do.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 22, 2026
@Charlesthebird
Charlesthebird added this pull request to the merge queue Sep 22, 2026
Merged via the queue into kagent-dev:main with commit cbc3940 Sep 22, 2026
29 checks passed
EItanya pushed a commit that referenced this pull request Sep 23, 2026
…lt UI image (#2874)" (#2910)

---
*🤖 written by Claude (start)*

This PR:
- Reverts #2874, removing the live Playwright CI job and restoring the
previous UI test layout.

## Testing
- Run `cd ui && yarn test:pw` on the restored suite.

```release-note
NONE
```

---
*🤖 written by Claude (end)*

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Signed-off-by: Nicholas Bucher <[email protected]>
Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
@toreysoloio
toreysoloio deleted the toreysoloio/uiLiveCi branch September 24, 2026 16:50
toreysoloio added a commit to toreysoloio/kagent that referenced this pull request Sep 24, 2026
kagent-dev#2874 was reverted for making test-e2e too long. It put the browser lane inside
that job, so its minutes landed in series on the job everything else waits on.

This restores the work and moves the lane out. test-e2e-ui stands up its own
cluster and runs beside test-e2e rather than after it, so a lane of about five
minutes hides inside one of about ten instead of extending it. test-e2e is
byte-identical to main again, ui.replicas=0 and five images included.

The second cluster costs a runner, not wall-clock. The job is also cheaper than
the one it left: three images where test-e2e builds five - controller, UI, and
one runtime for the harnesses to point at - because the harness kinds that job
needs differ in ways no browser journey can see.

It brings its own fixture rather than borrowing the Go suite's. Three harnesses,
all of one kind, so the template form still meets a cluster with more than one:
that is the branch where it applies no labels unasked, and a single-harness
fixture would have retired it silently. Verified by applying the fixture to a
dev cluster and running the suite against the three-harness shape - 13 passed,
including the admission journey.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Signed-off-by: Torey Scheer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Additional testing required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI: track the deferred Playwright coverage in DEFERRED.md [FEATURE] Run the live UI Playwright suite in CI against the built UI image

2 participants